fix: Store fixed-size strings inline in dd_core_config_t#320
Open
awforsythe wants to merge 1 commit into
Open
Conversation
refs: RUM-17284 ## Background In the C++ API, where we accept values like `env`, `version`, `service`, etc. as `std::string_view`, we implicitly copy the application-provided values into `std::string` members. In the C API, we need to hold string values in `dd_core_config_t` initially: when the core is created via `dd_core_create()`, we convert all of our config data to a C++ `CoreConfig` struct ## Problem The C API holds some string values as `char*` - we accept raw string pointers from the application and store them directly. This is fine for static strings, but it represents a use-after-free bug if the application-provided values have limited lifetime: we could crash on `dd_core_create()` if values supplied for client_token, env, version, service, etc., have temporary storage. ## Fix This PR fixes the problem by making `dd_core_config_t` (and its member struct `dd_internal_options_t`) store all string values inline, using fixed-size buffers. These fixed sizes are exposed as public constants: we already took this approach to prevent similar issues in the C logger API. If an application-provided string exceeds the expected maximum size, we truncate it and log a warning. This is a breaking ABI change (and it also renames a public constant in the C API, which technically makes it a breaking API change as well) that only affects the C API. Given the early stage of C API adoption, I think we should be fine to make this change swiftly. After the initial Flutter desktop pass came together, we briefly discussed making more fundamental changes to the way that config objects are allocated and stored in the C API. These changes are not in scope here: this PR is concerned only with preventing this specific class of bugs.
Contributor
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
refs: RUM-17284
Background
In the C++ API, where we accept values like
env,version,service, etc. asstd::string_view, we implicitly copy the application-provided values intostd::stringmembers.In the C API, we need to hold string values in
dd_core_config_tinitially: when the core is created viadd_core_create(), we convert all of our config data to a C++CoreConfigstructProblem
The C API holds some string values as
char*- we accept raw string pointers from the application and store them directly. This is fine for static strings, but it represents a use-after-free bug if the application-provided values have limited lifetime: we could crash ondd_core_create()if values supplied for client_token, env, version, service, etc., have temporary storage.Fix
This PR fixes the problem by making
dd_core_config_t(and its member structdd_internal_options_t) store all string values inline, using fixed-size buffers. These fixed sizes are exposed as public constants: we already took this approach to prevent similar issues in the C logger API.If an application-provided string exceeds the expected maximum size, we truncate it and log a warning.
This is a breaking ABI change (and it also renames a public constant in the C API, which technically makes it a breaking API change as well) that only affects the C API. Given the early stage of C API adoption, I think we should be fine to make this change swiftly.
After the initial Flutter desktop pass came together, we briefly discussed making more fundamental changes to the way that config objects are allocated and stored in the C API. These changes are not in scope here: this PR is concerned only with preventing this specific class of bugs.